Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[DEV] switch nose with pytest #18025

Merged
merged 4 commits into from
Apr 23, 2020
Merged

[DEV] switch nose with pytest #18025

merged 4 commits into from
Apr 23, 2020

Conversation

szha
Copy link
Member

@szha szha commented Apr 11, 2020

Description

switch nose with pytest

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented
  • To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • switch nose with pytest

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@mxnet-bot
Copy link

Hey @szha , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [centos-gpu, edge, windows-cpu, website, centos-cpu, unix-cpu, unix-gpu, windows-gpu, miscellaneous, sanity, clang]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@szha szha force-pushed the pytest branch 3 times, most recently from 2c73dcf to ccf2342 Compare April 11, 2020 02:58
@leezu
Copy link
Contributor

leezu commented Apr 11, 2020

How about the random seed handling and reproducibility? I think you'll need to adapt https://github.com/dmlc/gluon-nlp/blob/v0.9.x/conftest.py#L41

@szha szha force-pushed the pytest branch 3 times, most recently from d09779c to b7ab4a2 Compare April 11, 2020 06:53
@szha
Copy link
Member Author

szha commented Apr 11, 2020

@leezu thanks. Added.

@szha szha requested a review from leezu as a code owner April 11, 2020 20:22
@szha szha force-pushed the pytest branch 14 times, most recently from f94b92f to e848924 Compare April 15, 2020 23:05
leezu
leezu reviewed Apr 16, 2020
View reviewed changes
.github/workflows/os_x_staticbuild.yml
@@ -10,16 +10,16 @@ jobs:
uses: actions/checkout@v2
- name: Install Dependencies
run: |
brew install nasm automake ninja libtool cmake pkgconfig protobuf
brew install nasm automake ninja libtool cmake pkgconfig protobuf hdf5 zlib
python3 -m pip install --user -r ci/docker/install/requirements
Copy link
Contributor

@leezu leezu Apr 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest not to use ci/docker/install/requirements here but list the dependencies explicitly. Most of the things in ci/docker/install/requirements are not needed for this test and having a single requirements file that is used from an opaque number of locations for an variety of purposes makes it very hard to maintain that file.

Specifically, here we can just replace python3 -m pip install --user nose nose-timer nose-exclude numpy scipy with python3 -m pip install --user pytest numpy scipy a few lines below.

@marcoabreu
Copy link
Contributor

@mxnet-bot run ci [unix-gpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-gpu]

@leezu
Copy link
Contributor

leezu commented Apr 22, 2020

@szha there is a bug with the unix-gpu dist kvstore tests

Runtime on master is 6 minutes http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-gpu/detail/master/1887/pipeline/426 but more than 1 hour in this PR http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-gpu/detail/PR-18025/56/pipeline

@leezu
Copy link
Contributor

leezu commented Apr 22, 2020

Ok, the difference is due to hanging issue..

[2020-04-22T17:06:49.477Z] AssertionError: (None, array([[0.6, 0.6, 0.6],

[2020-04-22T17:06:49.477Z] [0.6, 0.6, 0.6]], dtype=float32), 41.0)

[2020-04-22T17:06:49.477Z] terminate called without an active exception

[2020-04-22T17:06:49.477Z] terminate called without an active exception

[2020-04-22T17:06:49.477Z] terminate called without an active exception

[2020-04-22T17:06:49.477Z] terminate called without an active exception

Thanks @marcoabreu to retrigger

@marcoabreu
Copy link
Contributor

The stability of our test suite is really a pain in the butt :(

You're welcome

@leezu
Copy link
Contributor

leezu commented Apr 22, 2020

This does not seem to be a stability issue, but a bug. Same failure again:

[2020-04-22T19:21:39.589Z] Traceback (most recent call last):

[2020-04-22T19:21:39.589Z]   File "dist_device_sync_kvstore.py", line 130, in <module>

[2020-04-22T19:21:39.589Z]     test_sync_push_pull()

[2020-04-22T19:21:39.589Z]   File "dist_device_sync_kvstore.py", line 88, in test_sync_push_pull

[2020-04-22T19:21:39.589Z]     check_default_keys(kv, my_rank, nworker, nrepeat=3)

[2020-04-22T19:21:39.589Z]   File "dist_device_sync_kvstore.py", line 72, in check_default_keys

[2020-04-22T19:21:39.589Z]     check_diff_to_scalar(val, num)

[2020-04-22T19:21:39.589Z]   File "dist_device_sync_kvstore.py", line 29, in check_diff_to_scalar

[2020-04-22T19:21:39.589Z]     assert(np.sum(np.abs((A - x).asnumpy())) == 0), (rank, A.asnumpy(), x)

[2020-04-22T19:21:39.589Z] AssertionError: (None, array([[0.6, 0.6, 0.6],

[2020-04-22T19:21:39.589Z]        [0.6, 0.6, 0.6]], dtype=float32), 41.0)

@szha

@leezu
Copy link
Contributor

leezu commented Apr 22, 2020

Still

[2020-04-22T21:11:19.922Z] + cd /work/mxnet/tests/nightly

[2020-04-22T21:11:19.922Z] + ./test_distributed_training-gpu.sh

[2020-04-22T21:11:41.844Z] worker 0 is initialized

[2020-04-22T21:11:41.844Z] worker 0 passed test_gluon_trainer_type

[2020-04-22T21:11:41.844Z] worker 2 is initialized

[2020-04-22T21:11:41.844Z] worker 2 passed test_gluon_trainer_type

[2020-04-22T21:11:41.844Z] worker 1 is initialized

[2020-04-22T21:11:41.844Z] worker 1 passed test_gluon_trainer_type

[2020-04-22T21:11:41.844Z] worker 3 is initialized

[2020-04-22T21:11:41.844Z] worker 3 passed test_gluon_trainer_type

[2020-04-22T21:11:41.844Z] terminate called without an active exception

[2020-04-22T21:11:41.844Z] terminate called without an active exception

[2020-04-22T21:11:41.844Z] terminate called without an active exception

[2020-04-22T21:11:41.844Z] terminate called without an active exception

yzhliu
yzhliu approved these changes Apr 23, 2020
View reviewed changes
Copy link
Member

@yzhliu yzhliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to me

@szha szha merged commit faccd91 into apache:master Apr 23, 2020
@szha szha deleted the pytest branch April 23, 2020 06:59
@marcoabreu
Copy link
Contributor

Woohoo!

This was referenced Apr 23, 2020
Closed
Merged
Merged
@haojin2 haojin2 mentioned this pull request Apr 30, 2020
Open
ChaiBapchya
ChaiBapchya reviewed Apr 30, 2020
View reviewed changes
ci/docker/runtime_functions.sh

# https://github.com/apache/incubator-mxnet/issues/11801
# if [[ ${mxnet_variant} = "cpu" ]] || [[ ${mxnet_variant} = "mkl" ]]; then
# integrationtest_ubuntu_cpu_dist_kvstore
# fi

if [[ ${mxnet_variant} = cu* ]]; then
$nose_cmd $NOSE_TIMER_ARGUMENTS --verbose tests/python/gpu
pytest --durations=50 --verbose tests/python/gpu
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we showing top 50 slowest duration only?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? The others are not really of interest

@RuRo RuRo mentioned this pull request May 27, 2020
Merged
@MoisesHer MoisesHer mentioned this pull request Jun 23, 2020
Merged
AntiZpvoh pushed a commit to AntiZpvoh/incubator-mxnet that referenced this pull request Jul 6, 2020
@szha @AntiZpvoh
[DEV] switch nose with pytest (apache#18025)
adbdc47 
* switch nose with pytest

* switch centos python to 3.6

* disable dist kvstore tests

* skip hanging test
@leezu leezu mentioned this pull request Jul 25, 2020
Merged
@leezu leezu mentioned this pull request Oct 12, 2020
Merged
@waytrue17 waytrue17 mentioned this pull request Mar 23, 2021
Merged
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@leezu leezu leezu left review comments

@ChaiBapchya ChaiBapchya ChaiBapchya left review comments

@marcoabreu marcoabreu marcoabreu left review comments

@yzhliu yzhliu yzhliu approved these changes

@aaronmarkham aaronmarkham Awaiting requested review from aaronmarkham aaronmarkham is a code owner

@eric-haibin-lin eric-haibin-lin Awaiting requested review from eric-haibin-lin eric-haibin-lin is a code owner

@gigasquid gigasquid Awaiting requested review from gigasquid gigasquid is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@szha @mxnet-bot @leezu @marcoabreu @yzhliu @ChaiBapchya